feat(network): use Retry-After header for HTTP 429 responses #15463
Conversation
rustbot
added
A-networking
src/cargo/util/network/retry.rs
Outdated
| @@ -47,6 +47,7 @@ use anyhow::Error; | |||
| use rand::Rng; | |||
| use std::cmp::min; | |||
| use std::time::Duration; | |||
| use time::OffsetDateTime; | |||
There was a problem hiding this comment.
I feel like we are on the way of dropping time? #15293
There was a problem hiding this comment.
Maybe using jiff isn't the right call?
In my testing, it can take 2 to 10 seconds for jiff to initialize.
I suspect part of the problem is that jiff is configured to use the system timezone data, and it's probably loading and initializing a bunch of junk. Unfortunately because gix-date enables the default features, we can't easily disable that AFAIK.
Can we maybe get gix-date to not use the default? I don't fully understand what all the jiff features do, or when or if they are necessary.
There was a problem hiding this comment.
It seems like jiff init is taking around that time in CI, which I agree is unacceptable. I'm not seeing the delay on Windows, but that's likely because jiff isn't using system timezone data there.
There was a problem hiding this comment.
I wonder if there's a way to do the parsing without loading the timezone data. The HTTP Date header is always GMT.
This only applies for things that use |
|
Would it be possible to add a test using our |
src/cargo/util/network/retry.rs
Outdated
| let expected = jiff::Zoned::now() | ||
| .until( | ||
| &jiff::civil::date(2100, 1, 1) | ||
| .at(0, 0, 0, 0) | ||
| .to_zoned(jiff::tz::TimeZone::UTC) | ||
| .unwrap(), | ||
| ) | ||
| .unwrap() | ||
| .total(jiff::Unit::Millisecond) | ||
| .unwrap() as u64; | ||
| let actual = Retry::parse_retry_after(&headers).unwrap(); | ||
| let diff = expected.abs_diff(actual.into()); | ||
| assert!((diff < 1000), "{} != {} ({})", expected, actual, diff); |
There was a problem hiding this comment.
Independent of the problem with jiff initialization, I would recommend moving the expected initialization to after the actual line. That should reduce the time variance (since computing expected is very efficient, whereas parse_retry_after is not).
There was a problem hiding this comment.
I'll change parse_retry_after to take in now as a parameter to simplify the testing and eliminate the variation.
There was a problem hiding this comment.
Correction, I think I had that backwards. It looks like jiff::Zoned::now() is what is taking all the time.
There was a problem hiding this comment.
Can you try again with latest push and see if it's fixed?
There was a problem hiding this comment.
I reported this upstream at BurntSushi/jiff#366.
What does this PR try to resolve?
Cargo registries that return HTTP 429 when the service is overloaded expect the client to retry the request automatically after a delay. Cargo currently does not retry for HTTP 429.
What changed?
In this implementation, the maximum delay is limited to Cargo's existing limit of 10 seconds. We could consider increasing that limit for this case, since the server is explicitly requesting the delay.